Repository navigation
Refactor Project Controller Tests - #438
Conversation
sbatson5
left a comment
sbatson5
left a comment
There was a problem hiding this comment.
Just 1 suggestion. This is looking great 👍
| assert data["attributes"]["title"] == "Test project" | ||
| assert data["attributes"]["description"] == "Test project description" | ||
| assert data["attributes"]["long-description-markdown"] == "A markdown **description**" | ||
| assert data["relationships"]["organization"]["data"]["id"] == Integer.to_string(project.organization_id) |
There was a problem hiding this comment.
This is what I love the most about these.
| @@ -30,24 +21,12 @@ defmodule CodeCorps.ProjectControllerTest do | |||
| project_1 = insert(:project, title: "Test Project 1", organization: organization) | |||
| project_2 = insert(:project, title: "Test Project 2", organization: organization) | |||
There was a problem hiding this comment.
Are the titles here necessary? Thoughts on reducing this to insert_pair and letting the factory generate the titles? Since we aren't testing for them, I'm not sure we need them to be unique.
joshsmith
left a comment
joshsmith
left a comment
There was a problem hiding this comment.
Some changes here. Remember to ask yourself where the dividing line is on what you're testing.
| data = | ||
| conn | ||
| |> json_response(200) | ||
| |> Map.get("data") |
There was a problem hiding this comment.
The spacing on this is off now that data was removed. Make sure to watch your alignment.
| slug: "test-project") | ||
| project = insert(:project, title: "Test project", slug: "test-project") | ||
|
|
||
| conn = get conn, "/test-organization/test-project" |
There was a problem hiding this comment.
So this is confusing and brittle because we're relying on test-organization from the factory, but specifying test-project.
I think I would rather do:
organization = insert(:organization)
project = insert(:project, organization: organization)
path = "#{organization.slug}/#{project.slug}"What do you think?
There was a problem hiding this comment.
That's much better!!
|
|
||
| describe "create" do | ||
| @tag :authenticated | ||
| @tag authenticated: :admin |
There was a problem hiding this comment.
This should just be @tag :authenticated if you're relying on insert(:organization_membership, role: "admin"), otherwise you're going to defer to the site admin authentication since it takes precedence.
| path = conn |> project_path(:create) | ||
| assert conn |> post(path, payload) |> json_response(403) | ||
| attrs = @invalid_attrs |> Map.merge(%{organization: organization}) | ||
| assert conn |> request_create(attrs) |> json_response(403) |
There was a problem hiding this comment.
Why are you doing this extra work here? Would request_create on its own not work?
There was a problem hiding this comment.
I was having trouble getting this test to pass and this was the only solution that worked. I couldn't figure out why the usual request_create wasn't working so I just included the attributes. I'll look at it again today.
There was a problem hiding this comment.
Here's the error I'm getting: ** (FunctionClauseError) no function clause matching in CodeCorps.Helpers.Policy.get_membership/2
There was a problem hiding this comment.
Okay we'll dig into it when we pair.
| test "renders 403 when not authorized", %{conn: conn} do | ||
| organization = insert(:organization) | ||
| attrs = @invalid_attrs |> Map.merge(%{organization: organization}) | ||
| assert conn |> request_update(attrs) |> json_response(403) |
There was a problem hiding this comment.
Same as above, why not just request_update?
| @@ -30,24 +21,12 @@ defmodule CodeCorps.ProjectControllerTest do | |||
| project_1 = insert(:project, title: "Test Project 1", organization: organization) | |||
| project_2 = insert(:project, title: "Test Project 2", organization: organization) | |||
amyschools
added
awaiting review
and removed
needs submitter response
awaiting review
labels
9e1587b to
1cf4a86
Compare
What's in this PR?
Add api_case.ex helpers to Project controller tests.
References
Closes #437
Progress on: #413